Render lock: keep a patch load's flush out of an in-progress render by dpwe · Pull Request #1205 · shorepine/amy

dpwe · 2026-10-01T02:18:00Z

Fixes a second patch-reload race that #1190 didn't cover. A patch load can free oscillators on the sending thread while the render thread is reading them.

The race. Before patches_load_patch(), amy_event_to_deltas_queue() calls flush_due_deltas() on whichever thread sent the event. The flush is needed so a queued reset lands before the load rebuilds the synth tables (24adfbc). It can run frees: a released voice's FREE_OSC, or amy_reset_oscs() for a reset. The render thread holds no lock while it renders, so those frees can hit oscs that amy_render / render_* / hold_and_modify are reading.

With one thread rendering and another reloading synth 1 back to back, main crashes under AddressSanitizer in about 6 runs of 10. Every crash is in the render path, as a heap-use-after-free or a NULL synth[osc].

The fix. A second lock, the render lock, always taken before the queue lock:

The render thread holds it for each whole block: flush, render and mix. That covers amy_simple_fill_buffer() (desktop, web, Godot, pyamy) and both ESP render paths in i2s.c. On ESP it's released before the I2S write, so a waiting load gets in while the fill task waits on DMA.
A patch load holds it only for its flush. The load itself runs unlocked, as before.
amy_execute_deltas() takes it around its flush, which covers the sample-transfer start in parse.c that flushes off the render thread.
Ordinary events (note-ons, parameter changes, CCs, sequencer edits) still take only the queue lock, so they never wait on a render.
Recursion: the render lock is recursive per thread, using a thread-local depth. A render-thread hook or a sequenced message can load a patch mid-block.
Lock code: each platform's lock primitive is now defined once and used for both locks.

Why only the flush. A first version held the render lock across the whole load. Measured on an AMYboard (ESP32-S3) with a 5.8 ms block and a ~35 ms DMA ring, that stalled the render badly. With the lock narrowed to the flush, the same test shows:

Load	Lock hold, whole load → flush only	Longest render wait	Longest block
Juno 1, 6 voices	19–30 ms → 9–11 µs	~10–11 ms → ≤ 8 µs	15–18 ms → 3.9–4.9 ms
DX7 130, 6 voices	47–71 ms → 8–13 µs	36–37 ms → ≤ 19 µs	43–46 ms → 4.3–5.4 ms
Grow to 8 voices	37–88 ms → 9–15 µs	27–29 ms → ≤ 11 µs	34–37 ms → 3.6–4.8 ms
Drum kit 258	12–13 ms → 8–10 µs	4–6 ms → ≤ 13 µs	8–12 ms → 3.6–5.4 ms

With the narrowed lock, no block waited more than 100 µs for it, and every 500 ms window produced its full 86–87 blocks. That held with background render at ~74% of the block. Loads still take 25–94 ms on the sending thread, as on main, but the render no longer waits for them. The instrumentation (-DAMY_LOCK_TIMING) and the LockTiming sketch are on claude/render-lock-timing; they're not part of this PR.

Not covered:

A reset that comes due partway through a load can still clear that load's bookkeeping. That's the same exposure main has. Protecting it would mean holding the render off for the length of the load, which the table rules out.
tulipcc's native Tulip/AMYboard render loops don't go through amy_simple_fill_buffer() or i2s.c. They need the same amy_grab_render_lock() / amy_release_render_lock() around each block to get the fix.
Pico and Teensy have no lock implementation (the no-op branch), so nothing changes there.
On ESP multicore, code on the second-core render task (esp_render_task) must not load a patch: it would wait on the fill task's render lock while the fill task waits for it. Nothing in AMY does this.

Testing

AddressSanitizer stress, 10 runs × 3000 back-to-back reloads: 10 clean with this branch (after merging current main), against 4 clean and 6 render-path crashes on main.
ThreadSanitizer: no lock-order inversions. The only data race left is the sending thread reading total_samples without a lock in amy_sysclock64(), which main has too.
make ctest passes. make test gives the same 90 pass / 43 fail as main in my environment; the failures are small numeric differences against the reference audio and fail identically on main.
AMYboard timing as in the table above.
Not yet run on the ESP32-P4 that hit the original crash. The check there is tools/p4_reload_stress with -DSTRESS_MODE=1 (on branch claude/p4-reload-stress), on main and on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UW4jqUQ4Xc1sdoqzUPL2YV

Generated by Claude Code

A patch load settles due deltas on the sending thread (flush_due_deltas before patches_load_patch, so a queued reset lands before the load rebuilds the synth tables). That flush can free oscs -- a released voice's FREE_OSC, or amy_reset_oscs() for a reset -- while the render thread is reading them, because rendering never held a lock. Desktop AddressSanitizer stress (one thread rendering, another reloading synth 1 back to back) crashes in amy_render/render_*/hold_and_modify on main in about 6 runs of 10. Add a second lock, the render lock, taken before the queue lock: - the render thread holds it for a whole block: flush, render, mix (amy_simple_fill_buffer, and the ESP fill task and single-thread path; released before the i2s write, so a load gets in while we wait on DMA); - a patch load holds it from its flush to the end of patches_load_patch, which also keeps a reset run by the render thread's flush from landing halfway through the load's bookkeeping or its clone-on-grow snapshot; - amy_execute_deltas takes it too, covering callers outside a render loop. Ordinary events only take the queue lock, so note-ons never wait for a render. The render lock is recursive per thread via a thread-local depth (a patch string can load a patch: drum kits open with `if3iv1in38Z`), and each platform's lock primitive is now defined once and used for both locks. Not covered: Pico/Teensy have no lock implementation (unchanged no-ops), and tulipcc's native Tulip/AMYboard render loops need the same wrap. Stress test, 10 runs x 3000 reloads under ASan: main 4 clean / 6 render crashes; this change 10 clean. make ctest passes; make test gives the same 90 pass / 43 fail as main here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UW4jqUQ4Xc1sdoqzUPL2YV

Measured on an AMYboard (ESP32-S3) with AMY_LOCK_TIMING: holding the render lock across patches_load_patch() held off the render for most of the load, and loads take tens of ms of CPU there: load flush hold render stall longest block Juno 1, 6v 6-9 us 19-30 ms 10-11 ms 15-18 ms DX7 130, 6v 6-8 us 47-71 ms 36-37 ms 43-46 ms grow to 8v 6-9 us 37-88 ms 27-29 ms 34-37 ms drum kit 258 6-8 us 12-13 ms 4-6 ms 8-12 ms against a 5.8 ms block and a ~35 ms DMA ring, so a DX7 load ran the ring dry. The flush is the part that frees oscs; it takes microseconds. Hold the render lock only around it, in the patch-load path and in amy_execute_deltas() (not across the sequencer tick, which can itself load a patch). The render thread still holds it for each whole block, so a flush still can't free oscs mid-render. This gives up protecting the load's bookkeeping from a reset that comes due mid-load (the same exposure main has); that needs a fix that doesn't hold the render off for the length of a load. ASan stress, 10 runs x 3000 reloads: 10 clean (main: 4 clean, 6 render crashes). make ctest passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UW4jqUQ4Xc1sdoqzUPL2YV

dpwe · 2026-10-01T02:49:42Z

I'm pretty much entirely relying on Claude for the logic and the solution here. It was verified by tests on AMYboard -checking both that it didn't block normal operation, and that it fixed the osc-freed-mid-load which was the original bug addressed in #1185.

dpwe · 2026-10-01T03:22:17Z

HW CI Bench appears to be blocked; I got Claude to run the same test on my local amyboard which passed and yielded these timings:

notes held	main (µs)	PR (µs)	Δ
1	1002	997	−5
2	1159	1149	−10
3	1727	1724	−3
4	1900	1887	−13
5	2503	2491	−12
6	2624	2616	−8

bwhitman · 2026-10-01T03:22:59Z

⛓️ tulipcc integration PR opened

This merge was pinned into tulipcc for full-system CI: shorepine/tulipcc#1383

Test it there and merge that PR to move tulipcc onto this AMY.

github-actions · 2026-10-01T13:50:07Z

🎛️ AMY HW CI (AMYboard bench)

Flashed this PR's AMY (LoadTestChord: 6-voice Juno patch=1, one held note every 2 s) onto the physical AMYboard and measured the smoothed render load as the chord grows — back-to-back with the same sketch built at the PR's merge base, so Δ is this PR's own cost.

✅ PASS — the bench ran the test to completion.

notes held	main @ `f555bc7`	this PR	Δ
1	998	991	-7
2	1157	1149	-8
3	1725	1718	-7
4	1899	1893	-6
5	2494	2482	-12
6	2612	2602	-10

Full chord settled render μs: 2605 (was 2612, Δ -0.3%) (peak 2609, 39 samples)

⬇️ Artifacts: serial log · load trace · report

_{Self-hosted bench (amyboardci). FAIL means only that the test could not run — the load values are informational, with no threshold and no audio compare. See tools/arduino_loadsweep/.}

Brings in amy up to 1.2.190 (e153f7d): the render lock (shorepine#1205), chained oscs evaluated tail-first (shorepine#1200), MIDI CC output and midi_cc naming an AMY parameter directly (shorepine#1175), a PCM render gain ramp (shorepine#1208), no osc allocation on the ingest path, and the docs that go with them. One conflict, in src/amy.c: upstream grew its own lock abstraction for ESP_PLATFORM (a FreeRTOS mutex behind lock_init/lock_take/lock_give) where we had a bare SemaphoreHandle_t plus esp_rom_sys.h. Took upstream's side, and dropped our a84e093 render-in-flight counter with it: upstream's render lock (f11a552, 2812743) closes the same FREE_OSC-under-render race more thoroughly -- the render thread holds it for a whole block and a patch load holds it across its flush -- and it was measured, where ours spun for up to 50 ms. Nothing else of ours changed; src/amy.c is now identical to upstream/main. f11a552 lists tulipcc's native render loops as needing the wrap themselves, but they do not: every tulipcc path reaches a render through an amy entry point that already takes the lock. The Tab5 sets platform.multithread = 0, so its audio task's amy_render_audio() takes the branch that grabs it around esp_render_on_cores() + amy_fill_buffer(); Tulip CC keeps the default multithread = 1 and renders in amy's own esp_fill_audio_buffer_task; AMYboard-in-VCV pulls blocks with amy_simple_fill_buffer(). All three are wrapped upstream. make ctest: all passed. make test: 135 tests pass, no failures, including our TestAllNotesOffNoteZero at err=-100.0 dB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude added 3 commits September 29, 2026 02:05

Merge remote-tracking branch 'origin/main' into claude/render-lock

9454012

dpwe merged commit 8e285d7 into main Oct 1, 2026
12 checks passed

bwhitman mentioned this pull request Oct 1, 2026

Pin amy to 8e285d7c0 (amy#1205) shorepine/tulipcc#1383

Closed

This was referenced Oct 3, 2026

Sample-transfer start runs the sequencer tick on the sending thread #1207

Closed

Sample-transfer start: flush due deltas only, not the sequencer tick #1210

Merged

Navigation Menu

Repository navigation

Render lock: keep a patch load's flush out of an in-progress render - #1205

Render lock: keep a patch load's flush out of an in-progress render#1205
dpwe merged 3 commits into
mainfrom
claude/render-lock

dpwe commented Oct 1, 2026

Uh oh!

dpwe commented Oct 1, 2026

Uh oh!

dpwe commented Oct 1, 2026

Uh oh!

Uh oh!

bwhitman commented Oct 1, 2026

Uh oh!

github-actions Bot commented Oct 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dpwe commented Oct 1, 2026

Uh oh!

dpwe commented Oct 1, 2026

Uh oh!

dpwe commented Oct 1, 2026

Uh oh!

Uh oh!

bwhitman commented Oct 1, 2026

⛓️ tulipcc integration PR opened

Uh oh!

github-actions Bot commented Oct 1, 2026

🎛️ AMY HW CI (AMYboard bench)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants